Skip to content

ref(core): Streamline ACS lookups #14453

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

ref(core): Streamline ACS lookups #14453

wants to merge 3 commits into from

Conversation

mydea
Copy link
Member

@mydea mydea commented Nov 25, 2024 •
edited
Loading

Safe a tiny amount of bytes by de-duplicating the lookup for ACS implementations.

@mydea mydea self-assigned this Nov 25, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 25, 2024 •
edited
Loading

size-limit report 📦

Path Size % Change Change
@sentry/browser 23.02 KB +0.05% +10 B 🔺
@sentry/browser - with treeshaking flags 21.72 KB +0.04% +8 B 🔺
@sentry/browser (incl. Tracing) 35.6 KB -0.06% -20 B 🔽
@sentry/browser (incl. Tracing, Replay) 72.39 KB -0.02% -14 B 🔽
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 62.68 KB -0.02% -8 B 🔽
@sentry/browser (incl. Tracing, Replay with Canvas) 76.69 KB -0.02% -8 B 🔽
@sentry/browser (incl. Tracing, Replay, Feedback) 89.18 KB +0.01% +9 B 🔺
@sentry/browser (incl. Feedback) 39.77 KB +0.04% +13 B 🔺
@sentry/browser (incl. sendFeedback) 27.66 KB +0.08% +21 B 🔺
@sentry/browser (incl. FeedbackAsync) 32.44 KB +0.05% +14 B 🔺
@sentry/react 25.72 KB +0.04% +8 B 🔺
@sentry/react (incl. Tracing) 38.48 KB - -
@sentry/vue 27.19 KB -0.03% -8 B 🔽
@sentry/vue (incl. Tracing) 37.42 KB -0.03% -8 B 🔽
@sentry/svelte 23.18 KB +0.02% +3 B 🔺
CDN Bundle 24.2 KB +0.08% +19 B 🔺
CDN Bundle (incl. Tracing) 37.17 KB -0.05% -19 B 🔽
CDN Bundle (incl. Tracing, Replay) 71.94 KB -0.02% -9 B 🔽
CDN Bundle (incl. Tracing, Replay, Feedback) 77.3 KB -0.01% -4 B 🔽
CDN Bundle - uncompressed 71.16 KB +0.02% +11 B 🔺
CDN Bundle (incl. Tracing) - uncompressed 110.47 KB -0.08% -86 B 🔽
CDN Bundle (incl. Tracing, Replay) - uncompressed 223.27 KB -0.04% -86 B 🔽
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed 236.48 KB -0.04% -86 B 🔽
@sentry/nextjs (client) 38.69 KB -0.05% -17 B 🔽
@sentry/sveltekit (client) 36.12 KB -0.01% -3 B 🔽
@sentry/node 134.63 KB -0.01% -10 B 🔽
@sentry/node - without tracing 96.47 KB +0.01% +3 B 🔺
@sentry/aws-serverless 106.69 KB -0.01% -4 B 🔽

View base workflow run

@mydea mydea marked this pull request as ready for review November 25, 2024 10:39
@mydea mydea requested review from chargome and lforst November 25, 2024 10:39
@codecov Codecov
Copy link

codecov bot commented Nov 25, 2024 •
edited
Loading

❌ 64 Tests Failed:

Tests completed Failed Passed Skipped
423 64 359 263
View the top 3 failed tests by shortest run time 
errors.test.ts Sends exception to Sentry
Stack Traces | 0.032s run time 
errors.test.ts:4:5 Sends exception to Sentry
transactions.test.ts Isolates requests
Stack Traces | 0.034s run time 
transactions.test.ts:117:5 Isolates requests
transactions.test.ts Captures request metadata
Stack Traces | 0.035s run time 
transactions.test.ts:139:5 Captures request metadata

To view more test analytics, go to the Test Analytics Dashboard
Got feedback? Let us know on Github

@mydea mydea force-pushed the fn/browser-no-acs branch from 07c0164 to 2063224 Compare November 25, 2024 12:12
@lforst
Copy link
Contributor

lforst commented Nov 25, 2024

Is this actually needed for a feature? To me this just looks like one more indirection to wrap my head around when reading the code. (Also looks like gzip is only marginally happy about this change - not sure if worth?)

@mydea
Copy link
Member Author

mydea commented Nov 25, 2024

Hmm lol, I do not understand how that adds bundle size in some scenarios, but at this rate it's not worth doing this I guess 😬

@mydea mydea closed this Nov 25, 2024
@mydea mydea deleted the fn/browser-no-acs branch November 26, 2024 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lforst lforst Awaiting requested review from lforst

@chargome chargome Awaiting requested review from chargome

Assignees

@mydea mydea

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mydea @lforst